Skip to content

[security] fix: reject empty env allow wildcards#78

Merged
steipete merged 1 commit into
openclaw:mainfrom
Hinotoi-agent:fix/reject-empty-env-wildcard
May 11, 2026
Merged

[security] fix: reject empty env allow wildcards#78
steipete merged 1 commit into
openclaw:mainfrom
Hinotoi-agent:fix/reject-empty-env-wildcard

Conversation

@Hinotoi-agent

Copy link
Copy Markdown
Contributor

Summary

This PR hardens Crabbox's environment-variable forwarding boundary so a bare wildcard allow pattern cannot silently forward every local environment variable to a remote lease.

  • Rejects env.allow: ['*'] / empty-prefix wildcard patterns in the env allowlist matcher.
  • Preserves explicit exact matches such as CI and non-empty prefix wildcards such as PROJECT_*.
  • Adds regression coverage for both the low-level matcher and the full repo-local .crabbox.yaml config path.

Security issues covered

Issue Impact Severity
Bare env.allow wildcard forwards all local environment variables A malicious or compromised repo can use repo-local Crabbox config to forward local secrets such as API tokens, cloud credentials, and broker tokens into the remote command environment before the requested command runs. Critical

Before this PR

  • Crabbox automatically loaded repo-local crabbox.yaml / .crabbox.yaml.
  • env.allow values ending in * were treated as prefix wildcards.
  • The pattern * produced an empty prefix, and every environment-variable name has that prefix.
  • allowedEnv() copied all matching values from os.Environ().
  • Remote command construction then serialized those values into the SSH command prefix.

After this PR

  • Empty-prefix wildcards are ignored instead of matching every variable.
  • Existing explicit allow entries still work.
  • Existing non-empty prefix wildcards still work.
  • Tests cover both the matcher behavior and a repo-local .crabbox.yaml attempting env.allow: ['*'].

Why this matters

Crabbox's security docs describe environment forwarding as allowlisted and state that secrets should stay local unless explicitly allowed. Repo-local config is controlled by the repository being run, not necessarily by the operator. A repository-controlled env.allow: ['*'] therefore turns a convenience setting into a local secret exfiltration primitive.

Attack flow

malicious repository commits .crabbox.yaml with env.allow: ['*']
    -> operator runs crabbox from that repository
        -> Crabbox loads repo-local config and forwards all local env vars
            -> repo-controlled command can read/exfiltrate local secrets on the lease

Affected code

Issue Files
Empty-prefix wildcard matches every env var internal/cli/repo.go, internal/cli/config.go, internal/cli/run.go, internal/cli/ssh.go
Regression coverage internal/cli/ssh_test.go, internal/cli/config_test.go

Root cause

Bare env.allow wildcard forwards all local environment variables:

  • Direct cause: envAllowed() accepted any pattern ending in * and passed strings.TrimSuffix(pattern, "*") directly to strings.HasPrefix().
  • Boundary failure: for *, the prefix is empty, so every local environment variable matched, including secret-looking values the operator did not individually allow.

CVSS assessment

Issue CVSS v3.1 Vector
Repo-local bare env wildcard secret forwarding 9.1 Critical CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:C/C:H/I:H/A:N

Rationale:

  • The attack requires an operator to run Crabbox in a malicious or compromised repository, but no additional Crabbox privileges are required once that repo config is loaded.
  • Scope changes because repo-controlled config can cause local operator secrets to be disclosed to a remote lease environment controlled by the run.
  • Confidentiality impact is high because local cloud/API/broker tokens may be forwarded.
  • Integrity impact is high when forwarded tokens grant write access to external systems.

Safe reproduction steps

  1. In a Crabbox checkout before this fix, create a temporary repository directory containing:

    env:
      allow:
        - '*'
  2. Set a proof environment variable, for example:

    export CRABBOX_PROOF_API_TOKEN=critical-secret-value
  3. Load Crabbox config from inside that repository and inspect allowedEnv(cfg.EnvAllow).

  4. Observe that the proof variable is included even though it was not individually allowlisted.

  5. Build a remote command with remoteCommandWithEnvFile() and observe the proof variable serialized into the command prefix.

The regression test added in this PR performs the same check without contacting a remote provider.

Expected vulnerable behavior

  • envAllowed("CRABBOX_PROOF_API_TOKEN", []string{"*"}) returned true.
  • A repo-local .crabbox.yaml with env.allow: ['*'] caused allowedEnv(cfg.EnvAllow) to include the local proof token.
  • The generated remote command could include the proof token assignment.

Changes in this PR

  • Adds an empty-prefix guard to envAllowed() before applying prefix wildcard matching.
  • Keeps non-empty prefix wildcard behavior unchanged.
  • Adds matcher-level regression coverage for bare and whitespace-trimmed * patterns.
  • Adds config-level regression coverage for repo-local .crabbox.yaml with env.allow: ['*'].

Files changed

Category Files What changed
Env allowlist hardening internal/cli/repo.go Ignore wildcard patterns whose prefix is empty.
Matcher regression tests internal/cli/ssh_test.go Verify bare * no longer matches all variables and PROJECT_* still works.
Repo config regression tests internal/cli/config_test.go Verify repo-local env.allow: ['*'] does not forward a proof secret.

Maintainer impact

  • This is intentionally narrow: it only changes the pathological empty-prefix wildcard case.
  • Existing exact allow entries continue to work.
  • Existing non-empty prefix wildcard entries continue to work.
  • No provider, Worker, or remote execution behavior changes beyond the environment values admitted by the allowlist.

Fix rationale

A wildcard matcher should never treat an empty prefix as authorization to forward every local environment variable. Requiring wildcard prefixes to be non-empty keeps the documented allowlist behavior useful while preventing the broadest repo-controlled secret forwarding case.

Type of change

  • Security fix
  • Tests
  • Documentation update
  • Refactor with no behavior change

Test plan

  • Confirmed the new matcher regression fails on the vulnerable code path before the fix.
  • Ran focused CLI regression tests.
  • Ran the full Go test suite.
  • Ran Go vet.
  • Checked whitespace/diff hygiene.

Executed with:

go test ./internal/cli -run TestEnvAllowlistRejectsEmptyWildcardPrefix -count=1 -v
go test ./internal/cli -run 'TestEnvAllowlist|TestRepoConfigBareEnvWildcardDoesNotForwardEveryLocalVariable' -count=1 -v
go test ./internal/cli -count=1
go test ./... -count=1
go vet ./...
git diff --check

Token usage

  • discovery tokens: partial/unknown
  • validation tokens: partial/unknown
  • duplicate-check tokens: partial/unknown
  • PR/writeup tokens: partial/unknown
  • total tokens: partial/unknown
  • notes: exact token telemetry was not available in this local PR workflow.

Disclosure notes

  • This PR is bounded to the bare wildcard environment-forwarding path.
  • It does not claim to block every possible risky env.allow pattern; it removes the broad empty-prefix wildcard behavior while preserving explicit allowlist functionality.
  • No unrelated files were changed.

@steipete
steipete merged commit eaae40a into openclaw:main May 11, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants